Skip to content

process.stdin: leave the ref of the paused stdin source for its next reader - #43747

Closed
robobun wants to merge 3 commits into
claude/http-consolidatedfrom
robobun/be16bce1/stdin-pause-starves-web-readers
Closed

robobun wants to merge 3 commits into
claude/http-consolidatedfrom
robobun/be16bce1/stdin-pause-starves-web-readers

Conversation

@robobun

@robobun robobun commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #46528. It needs the resume that #46528 adds when a reader attaches.

Problem

  • After process.stdin.pause() or rl.close(), for await (const line of console) reads nothing and the process exits with code 0. A Bun.stdin.stream() reader and Readable.fromWeb(Bun.stdin.stream()) do the same.
  • http: consolidate the open fetch, Bun.serve, node:http, node:http2 and HTTP/3 pull requests #46528 fixes these only under a top-level await.
  • The cause is the last line of disown() (src/js/builtins/ProcessObjectInternals.ts:154), source?.updateRef?.(false). The next reader resumes the source, but its poll keeps nothing alive.

Fix

  • disown() ends in source?.updateRef?.(!forceUnref). That is the whole source change.
  • Correct because a paused source has no armed poll. The ref keeps nothing alive until a reader resumes the source, and that reader's release drops it.
  • Verified: 32 new tests in test/js/node/process/process-stdin.test.ts. 27 fail without the line. The other 5 check that the process still exits.
  • Self-reviewed: 8 concerns raised, 7 addressed (Notes).

Background

Downsides

  • A reader taken after the pause and never read keeps the process alive until stdin closes, as an idle reader does when nothing paused.
  • Still broken: a reader that holds the lock when pause() runs reads nothing.
  • No new call, allocation or syscall. The builtin text grows by 9 bytes.
Notes

What is left of this PR. Until b3356db this PR had three parts: a resume when a reader locks the stream (C++), a started check in FileReader::set_flowing (Rust), and two lines in disown(). #46528 has the first two since 44e4858, as resumeNativeSource() in setUpReadableStreamDefaultReader and if !self.started.get() || ... in set_flowing. They are gone from here, and so is the segfault of pause() followed by Readable.fromWeb(Bun.stdin.stream()), which #46528 fixes. One line of disown() is left.

Measured (Linux x64, debug builds of 04af03e with and without the line). The child runs the first column and then reads. The parent writes the input 500 ms after the child prints a marker, and then closes the pipe.

The child runs main 7d240c1 (release) #46528 #46528 with the line
pause(), one setImmediate, the console iterator nothing, exit 0 nothing, exit 0 reads all
the same with a getReader() loop, for await, pipeTo() or .text() nothing, exit 0 nothing, exit 0 reads all
the same with Readable.fromWeb(Bun.stdin.stream()) SIGSEGV nothing, exit 0 reads all
rl.close(), resume() then pause(), or a 'data' listener that pauses, then a reader nothing more, exit 0 nothing more, exit 0 reads all
top-level await, pause(), a reader that waits 1 s: CPU time never reads 979 to 1004 ms 10 to 11 ms

The last row: with nothing ref'd, a pending top-level await spins in EventLoop::wait_for_promise until the input arrives. await new Promise(() => {}) alone does the same on 1.3.13 (#14951), so that part is older and not changed here.

History: the pause() rows read everything on 1.3.13 and nothing on 1.4.2 (SIGSEGV for Readable.fromWeb). Since #34971 (1.4.0) a reader remembers an unref that comes before its first poll registration, and disown() unrefs a source that never started. The rl.close() rows read nothing on 1.3.13 too.

The tests. One describe block, four ways for process.stdin to let go (pause(), rl.close(), destroy(), a break out of for await (const chunk of process.stdin)) and three readers (the console iterator, a Bun.stdin.stream() reader, Readable.fromWeb). No child has a top-level await.

  • 12 rows: the reader reads the rest of stdin.
  • 12 rows: the reader reads one chunk and lets go. The parent leaves stdin open, and the child has to exit by itself.
  • 4 rows: nothing reads stdin again, stdin stays open, the child has to exit.
  • 1 row each: the same over a shell pipe (bun | bun), process.stdin takes stdin back after the reader, a readline prompt on a pty followed by the console iterator, and process.stdin.unref() before the pause. The writer of the pipe is not cat: busybox cat copies with sendfile(), and a sendfile() that waits for input keeps the pipe locked, so the reader blocks in read().

The child prints its marker from an unref'd 50 ms timer. That timer fires only while stdin keeps the process alive, so without the line the marker never comes and the parent never writes. On debug builds of 04af03e, 26 rows failed in 5 of 5 runs without the line and all rows passed in 5 of 5 runs with it. The shell pipe row failed in 3 of 3 runs without the line and passed in 3 of 3 with it (release builds). The 5 rows that pass both ways are the 4 exit rows and the unref() row. With updateRef(true) in place of the line, the unref() row fails.

Self-review. Concerns and what happened to each:

  1. The resume at lock time can run user JS (getReader() in a beforeExit listener runs a queued process.nextTick callback before it returns). This PR no longer has a resume. The same holds for resumeNativeSource in http: consolidate the open fetch, Bun.serve, node:http, node:http2 and HTTP/3 pull requests #46528, reported there.
  2. The lock check made pause() a no-op under every lock holder. Removed, see below.
  3. The rows raced the exit of the child. Now the marker comes from the unref'd timer.
  4. No row could fail on a leaked keep-alive. Now 17 rows leave stdin open.
  5. The forceUnref half had no test. Now the unref() row.
  6. The native.locked half had no test. Removed with the lock check.
  7. The 'close' path had no test. Now two of the four doors.
  8. Only socketpair and pty stdin. Added the shell pipe row. Not added: rows with stdin from a file. They pass without the line, because a regular file has no poll.

The lock check left this PR. if (!reader && native.locked) return; made process.stdin leave a source alone while another reader holds the lock. It is a new ownership rule with a cost: after pause() under a lock holder, a child with stdin: "inherit" got nothing in 5 of 10 runs (0 of 10 without the check), because the holder still reads fd 0. That is the contract of #13978 and #23333. It needs its own decision. #43951 points at the same case.

Not changed.

Suites. Debug build with the line, 0 failures: process-stdin.test.ts (63), console-iterator.test.ts (17), readline.node.test.ts (81), readline_promises.node.test.ts, pause_stdin_should_exit, readline_never_unrefs, stdin_fell_asleep, process-stdio.test.ts, process-stdin-stale-hup, terminal.test.ts (101), terminal-spawn.test.ts, test/js/node/tty, spawn-stdin-readable-stream.test.ts (54), spawn-socketpair-shutdown, spawn-streaming-stdin, node-stream.test.js (162), and the 33 Node test-stdin-*, test-tty-stdin-*, test-readline-* files. On this machine stdin-fixtures.test.ts (a 1 s kill timer) and stdin-pause-pty.test.ts fail on debug builds with and without the line, and pass 3 of 3 on release builds both ways. Nothing ran on macOS or Windows here.


no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/process/process-stdin.test.ts

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 736a54b and b3356db.

Files selected for processing: 4
  • src/js/builtins/ProcessObjectInternals.ts
  • src/jsc/bindings/webcore/streams/ReadableStreamOperations.cpp
  • src/runtime/webcore/FileReader.rs
  • test/js/node/process/process-stdin.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Walkthrough

The change updates native stdin flowing-state handling and process stdin reader ownership. It adds coverage for stdin handoff across pauses, readline closure, web-stream reads, and terminal input.

Changes

Stdin flowing ownership

Layer / File(s) Summary
Native source flowing control
src/jsc/bindings/webcore/streams/ReadableStreamOperations.cpp, src/runtime/webcore/FileReader.rs
Readable stream reader initialization calls a C ABI function to set a native source to flowing. FileReader::set_flowing updates the state and returns early if the reader has not started.
Process stdin reader ownership
src/js/builtins/ProcessObjectInternals.ts
disown leaves the flow and reader unchanged when another reader holds the native lock. After releasing its own reader, it sets the native source reference according to forceUnref.
Stdin lifecycle regression coverage
test/js/node/process/process-stdin.test.ts
Tests cover stdin handoff after pauses and readline closure, pending web-stream reads, resumed data listeners, and console iteration after terminal input.

Suggested reviewers: jarred-sumner

Priority: Normal

Merge Risk: Minimal · up to b3356

No identified issue remains that needs to be resolved before merging.

Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly identifies the main change: retaining the paused stdin source reference for a subsequent reader.
Description check Passed The description follows the repository template. It has both required sections, "What does this PR do?" and "How did you verify your code works?". The "What does this PR do?" part is detailed. It stat…
Full details: Description check

Explanation

The description follows the repository template. It has both required sections, "What does this PR do?" and "How did you verify your code works?". The "What does this PR do?" part is detailed. It states the problem, the one-line fix in disown(), the background on updateRef and forceUnref, the downsides, and the remaining unfixed cases. The verification part is also detailed. It names 32 new tests in process-stdin.test.ts, with 27 failing without the change. It lists measured results on Linux x64 and a long list of suites that pass on a debug build. It states that nothing ran on macOS or Windows. The description also states it is stacked on #46528 and that the PR is reduced to one line in disown(). This conflicts with the raw summary, which lists changes in ReadableStreamOperations.cpp and FileReader.rs. The description says those parts were removed, or moved to #46528, in commit b3356db. The PR objectives describe a different scope: a resume on reader lock, a started check, and a 9-test suite. So the description does not match the objectives or the file summaries. The check is about the template structure, so the mismatch is recorded but does not change the status. The description is long, and the Notes section is dense. That affects readability, not completeness.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: closed. The line of this PR (source?.updateRef?.(!forceUnref) at the end of disown()) and its tests are in #46528 since 0b04aed.

How it was reproduced: Linux x64, debug builds of the branch of #46528 (04af03e) with and without the line. A parent waits for a marker that the child prints from an unref'd timer, writes to the child's stdin, and closes the pipe.

  • After process.stdin.pause(), rl.close() or process.stdin.destroy(), the console iterator, a Bun.stdin.stream() reader and Readable.fromWeb(Bun.stdin.stream()) read nothing more. The child exits with code 0 before the marker. 27 of the 32 new tests fail without the line, and all 32 pass with it.
  • A release build of main 7d240c1 reads nothing in the same rows, and the Readable.fromWeb row ends with Segmentation fault at address 0x15C.

@Jarred-Sumner Jarred-Sumner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

terrible PR. does this really need a getter? does this really need another function?

@robobun robobun changed the title process.stdin: hand a paused stdin back to Bun's own readers Stop a native stream source when its reader releases, resume it on lock (fixes stdin after process.stdin pauses) Sep 22, 2026
@robobun robobun changed the title Stop a native stream source when its reader releases, resume it on lock (fixes stdin after process.stdin pauses) streams: stop a native source on reader release, resume it on lock Sep 22, 2026
@robobun

robobun commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

No, it needed neither. Reworked in 1d2ebbb. The diff is now 3 source files (+34, -26) and has no Rust change.

  • The locked getter is gone. disown() is now only reader.releaseLock().
  • The ref bookkeeping is gone: the previous-state return from updateRef(), the bit on the stream, the KEEP_ALIVE flag for the Windows reader, and the second call into the handle.
  • The rule is now: a reader release stops the native source (setFlowing(false)) in place of the unref, and a reader lock resumes it (setFlowing(true)). A stopped source does not keep the process alive, and its ref stays as the user set it, so the next reader has nothing to restore.
  • One static helper is left in ReadableStreamOperations.cpp. It replaces the inline block that the release path had, and the lock path shares it.

The lock-time call is the part that cannot go away. After rl.close() the stream already has a native pull parked, so a later read() never reaches FileReader::on_pull, and only the lock can resume the source.

The same rule also covers the second reader after a release (#41578, its 5 tests are here and pass with no reader change), a Bun.stdin.stream() reader that holds the lock when process.stdin.pause() runs, and a child process that inherits stdin after the console iterator lets go.

Both test files pass on Linux and Windows x64 with the debug build. 10 of the 10 new process-stdin.test.ts tests fail on 1.4.3. The behavior change to weigh is in Downsides: a file-backed stream with no reader no longer reads ahead.

@robobun

robobun commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:05 AM PT - Oct 24th, 2026

❌ @robobun, your commit c937323 has 1 failures in Build #132126 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 43747

That installs a local version of the PR into your bun-43747 executable, so you can run:

bun-43747 --bun

@robobun
robobun force-pushed the robobun/be16bce1/stdin-pause-starves-web-readers branch from c8e37d2 to d2925c8 Compare September 22, 2026 04:44
@robobun robobun changed the title streams: stop a native source on reader release, resume it on lock process.stdin: let the next reader of Bun.stdin.stream() resume a paused stdin Sep 22, 2026
@robobun

robobun commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

Update: the "release stops the source" shape from my last comment was wrong, and CI showed it. A pull that arrives after the release is parked in the stopped source and roots it, so streams-leak.test.ts (abandoned Bun.file().stream() reader) leaked its fd on every POSIX lane. I dropped that shape.

d2925c8 is the smallest fix I can find for the reported bug. 3 source files, +19, -1, no getter, no new function, no Rust change, and the release path is untouched:

  • readableStreamReaderGenericInitialize: a reader that locks a native-source stream calls setFlowing(true) on the source. After rl.close() a native pull is already parked, so a later read() never reaches FileReader::on_pull, and the lock is the only place that can resume the source. A source that flows ignores the call.
  • disown(): updateRef(false) becomes updateRef(!forceUnref). The source is stopped at that point, so the ref keeps nothing alive. It is there for the next reader's poll.

All 6 rows of the report read everything, also on a pty and with a pending top-level await. 8 new tests in process-stdin.test.ts, all fail on 1.4.3. streams-leak.test.ts, the stdin, readline, spawn and streams suites pass locally, and the two test files pass on Windows x64.

This shape does not cover #41578 (second reader after another reader's release), so I reopened that PR and took its tests out of this one.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings marked 🟡 are optional suggestions and need no follow-up push.

@robobun
robobun force-pushed the robobun/be16bce1/stdin-pause-starves-web-readers branch from d2925c8 to a5d5d29 Compare September 22, 2026 05:00
@robobun

robobun commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

a5d5d29: the lock-time resume no longer goes through getIfPropertyExists and a JS call. The handle is a generated native class, so the lock path downcasts it to JSFileInternalReadableStreamSource and calls FileReader__setFlowing(source->wrapped(), true), a new extern "C" export in FileReader.rs. The setFlowing builtin name is gone again. Source diff is now 4 files, +21, -1.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked the new lock-time resume in readableStreamReaderGenericInitialize: it runs after reader->m_stream / stream->m_reader are set, so a chunk or EOF that FileReader::set_flowing(true) dispatches synchronously via IOReader::read lands on a fully wired reader, and the uncheckedDowncast to JSNativeStreamSourceAdapter is guarded by the same SourceKind::Native check the existing release path uses.

Extended reasoning...

The PR was reworked since the prior review: the resume is now a direct C++ downcast to JSFileInternalReadableStreamSource at reader-lock time (ReadableStreamOperations.cpp, FileReader.rs, WebStreamsInternals.h) plus a one-line ref change in ProcessObjectInternals.ts and new process-stdin tests; no security-sensitive surface. A confirmed inline finding is being posted, one further verified finding was not posted, and Jarred-Sumner's CHANGES_REQUESTED review predates the new commit with no visible resolution, so approval is not appropriate.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased on main (the branch had conflicts in ReadableStreamOperations.cpp and FileReader.rs) and added one fix. The source diff is 3 files, +24, -1.

  • Rebase: the lock-time resume follows Bun__ByteBlobLoader__remaining in BunStreamSource.cpp. It downcasts the handle to Bindgen::Generated::JSFileInternalReadableStreamSource and calls Bun__FileReader__setFlowing(source->wrapped(), true). The declaration is next to its use, so WebStreamsInternals.h is out of the diff.
  • New in b3356db: process.stdin.pause(), one tick, then Readable.fromWeb(Bun.stdin.stream()) segfaults on main (Segmentation fault at address 0x15C, 3 of 3 runs of head -c 300000 /dev/zero | bun pause.mjs). disown() calls setFlowing(false) on a source that nothing started. The native path of Readable.fromWeb then calls setFlowing(true) before start(), and FileReader::set_flowing reads through a reader that has no parent yet. set_flowing now only records the flag before on_start. The crash was reported in http: consolidate the open fetch, Bun.serve, node:http, node:http2 and HTTP/3 pull requests #46528 (comment).

A release build of main fails the 11 new tests in process-stdin.test.ts. Release and debug builds of this branch pass all 33. A release build grows by 313 bytes of text.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

@robobun robobun changed the title process.stdin: let the next reader of Bun.stdin.stream() resume a paused stdin process.stdin: leave the ref of the paused stdin source for its next reader Oct 24, 2026
@robobun
robobun changed the base branch from main to claude/http-consolidated October 24, 2026 15:58
@robobun
robobun force-pushed the robobun/be16bce1/stdin-pause-starves-web-readers branch from b3356db to ce0a4b9 Compare October 24, 2026 15:58

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

…reader

disown() ended in source.updateRef(false). The next reader of
Bun.stdin.stream() resumes the source, but the poll that it arms did not
keep the process alive. A console iterator, a Bun.stdin.stream() reader
or a Readable.fromWeb() that started after process.stdin.pause(),
rl.close() or process.stdin.destroy() read nothing, and the process
exited with code 0, unless a top-level await kept it alive.

disown() now ends in source.updateRef(!forceUnref). A paused source has
no armed poll, so the ref keeps nothing alive until a reader resumes the
source. The release of that reader drops the ref.
busybox cat copies with sendfile(). A sendfile() that waits for input on
a socket keeps the output pipe locked, so the child blocks in read() on
bytes that are already in the pipe. The row timed out on Alpine.
@robobun
robobun force-pushed the robobun/be16bce1/stdin-pause-starves-web-readers branch from ce0a4b9 to 0156904 Compare October 24, 2026 16:28

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline note on run(), I traced the other two disown() callers through the native side: on the EOF path (internalRead -> disown with stream_reachedEof) and on 'close', the new updateRef(true) reaches PosixBufferedReader::update_ref, which only records KEEP_ALIVE and returns before touching a poll that is not watching, and FileReader::set_ref_unref is a no-op once the source is done, so neither path can hold the process open by itself. The unref() path still passes false through !forceUnref, and a later reader's releaseLock() drops the ref again via releaseNativeSourceRef, so the "exit after one chunk" rows exercise the real release sequence.

Extended reasoning...

The diff is one line in src/js/builtins/ProcessObjectInternals.ts (disown() now ends with source?.updateRef?.(!forceUnref)) plus a new describe block of 33 subprocess tests in test/js/node/process/process-stdin.test.ts; no security-sensitive surface. Not approving because a maintainer's CHANGES_REQUESTED review from the first version is still the latest human review state, the PR has since been reshaped (C++/Rust parts moved to a stacked PR), and whether the ref belongs in disown() versus reader attachment is a layering decision the maintainers should settle.

run() read stderr only after stdout ended, and not at all when stdout
ended before a marker. A child that failed early gave no reason, and a
child that filled the stderr pipe could not finish.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

@robobun

robobun commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: the line and the tests of this PR are in #46528 (0b04aed).

@robobun robobun closed this Oct 24, 2026
robobun added a commit that referenced this pull request Oct 26, 2026
A reader release only dropped the event-loop ref of the native file
source. The source of Bun.stdin.stream() kept its poll on fd 0 and took
input that arrived while no reader was attached: one parked pull, then
one more chunk. A dialog, fs.readSync(0) or a child process with
inherited stdin did not get that input. After a break out of
`for await (const line of console)` on a terminal, a child that reads
stdin got 1 to 5 of 8 typed lines.

The release hook now calls Bun__FileReader__onReaderRelease. A source
that reads stdin, stdout or stderr as a pipe, socket or terminal stops,
and the next reader lock resumes it through resumeNativeSource. A
stopped source keeps nothing alive, so it keeps its ref for that next
reader: a second console loop in an async function now waits for its
input. Every other source reads on and drops its ref as before: a
regular file, a path, another descriptor, a Bun.spawn pipe, and on
Windows a console.

A consumer that stops the source inside the delivery of a chunk ends
the read loop. The loop read once more after a chunk of 131,072 bytes
or more.

set_flowing(true) reads only when the buffer of the source is below
the highwater mark. Without that limit each release and lock adds one
chunk to the buffer.

The console iterator keeps the unfinished line at the end of the chunk
that it stopped in. The next loop lost that start of a line.

The rule "a reader release stops the native source, a reader lock
resumes it" is from d227cc7 in #23341 and from 1d2ebbb in #43747.
Both had no scope. The first stopped the file readers of Windows and
the stdout of a child. The second leaked the descriptor of an abandoned
Bun.file().stream() reader.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants